Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename squeeze -> dropdims #28303

Merged
merged 2 commits into from
Jul 28, 2018
Merged

Rename squeeze -> dropdims #28303

merged 2 commits into from
Jul 28, 2018

Conversation

Keno
Copy link
Member

@Keno Keno commented Jul 27, 2018

And one more of the items discussed on triage. squeeze is an awkward, non-descriptive function. We can do better. Going by the principle of looking at how the docstring describes it, suggests rmdims or dropdims. Of these dropdims seems clearest. Rename it.

2×2×1 Array{Int64,3}:
[:, :, 1] =
1 3
2 4
```
"""
squeeze(A; dims) = _squeeze(A, dims)
function _squeeze(A::AbstractArray, dims::Dims)
dropdims(A; dims) = _dropdims(A, dims)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not strictly related to this PR, but what's the value of having dims be a keyword argument for a simple two arg function? I know they are fast now, but here it doesn't seem needed. The rename makes this a bit more jarring, since we now have dropdims(A,dims=1) which is redundant.

I believe there are other functions where I've noticed this, though can't remember which exactly. I can open an issue if we want.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's just for consistency --- all functions that accept a list of dimensions accept them with a dims keyword argument, so it's easier to guess or remember how to call it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was done for consistency. All functions that operate on dimensions now take those as a keyword argument.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jinx!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I see now they were all changed. I'm not a huge fan, but I suppose that ship has sailed.

@ararslan ararslan added arrays [a, r, r, a, y, s] deprecation This change introduces or involves a deprecation labels Jul 27, 2018
@Keno Keno merged commit cb6f5e2 into master Jul 28, 2018
@ararslan ararslan deleted the kf/squeeze branch July 28, 2018 01:30
KristofferC added a commit that referenced this pull request Jul 28, 2018
galenlynch added a commit to galenlynch/Compat.jl that referenced this pull request Aug 16, 2018
`squeeze` was renamed `dropdims` in [#28303](JuliaLang/julia#28303)

Fixes JuliaLang#617
galenlynch added a commit to galenlynch/Compat.jl that referenced this pull request Aug 16, 2018
`squeeze` was renamed `dropdims` in [#28303](JuliaLang/julia#28303)

Fixes JuliaLang#617
galenlynch added a commit to galenlynch/Compat.jl that referenced this pull request Aug 19, 2018
`squeeze` was renamed `dropdims` in [#28303](JuliaLang/julia#28303)

Fixes JuliaLang#617
martinholters pushed a commit to JuliaLang/Compat.jl that referenced this pull request Aug 21, 2018
* squeeze is now dropdims

`squeeze` was renamed `dropdims` in [#28303](JuliaLang/julia#28303)

Fixes #617

* Support dropdims for dev versions where squeeze had KW arg

Also, remove README information about squeeze support, since dropdims is the
preferred replacement.

* Export dropdims
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrays [a, r, r, a, y, s] deprecation This change introduces or involves a deprecation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants